-
-
Notifications
You must be signed in to change notification settings - Fork 22
snowcap: add TextInput widget #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
snowcap: add TextInput widget #369
Conversation
33b380b to
756d3af
Compare
@Ottatop do you have thought on that ? Most The second point is only for types without a default implementation. The alternative is to hardcode our own default when the translation between protobuf & iced types fails. |
Agree, it would be best to crack down on these panics (especially if anyone wants to make protobuf bindings), though I strongly recommend doing this in another PR seeing as this one's already up to 2.5k lines |
Agree, I'll only try to remove the ones introduced by this PR and removable :) |
7998627 to
d4aa251
Compare
|
Allright, I think that's all. FYI, the PR is on top of #364. because I needed some of the changes from that PR, and it makes for a cleaner history that cherry-picking the commits I need (plus the eventual rebase hell if I discovered some issue in that PR). It should rebase cleanly should you choose to first pull the other one, and then rebase + merge this one (which could also be easier to review, I guess ?) OTOH, you can also close the other one and track everything here if you prefer. |
6944918 to
f4196da
Compare
| message Background { | ||
| oneof background { | ||
| Color color = 1; | ||
| Gradient gradient = 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having second thought on this.
It matches iced implementation, and will allow future addition of gradient without code duplication, but the current widget have a standalone background_color, which make me think the goal might've been to have a backround_gradient or a background_linear field instead.
I don't feel strongly about which solution ends up implemented, but IMO if this is merged as it is before 0.2 gets out, we should realign other widgets so they use this message for backgrounds and we don't have to duplicate the effort everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ottatop do you have an input on this ?
Snowcap protobuf v1 was added after after the 0.1, so changing the current way we handle background in container & button would not be a breaking change. However doing so later would.
I can extract this in a standalone PR if you want it.
9c32f92 to
153ca98
Compare
1fe3a6e to
3042cd0
Compare
3042cd0 to
3ef8c4e
Compare
3ef8c4e to
8d33651
Compare
The title say it all.
This PR adds TextInput widget, which will be useful as a building block for e.g. prompts.
On top of that, I've enabled support for key-repetition so the prompt themselves feel more natural.
TODO:
Postponed to [API] Gracefully handle protobuf translation issues #370Removed the ones from this PR.